Exit when json::parse_string failed, avoid double valid string check #1791
Conversation
| } | ||
|
|
||
| add_error(msg::format(msgUnexpectedEOFMidString)); | ||
| vcpkg::Checks::msg_exit_with_message(VCPKG_LINE_INFO, msgInvalidString); |
There was a problem hiding this comment.
This code is quite clearly trying to return the error message rather than terminate, I don't think elevating an error into termination is appropriate, particularly when this can happen from whatever user supplied input rather than a bug in the tool.
There was a problem hiding this comment.
Currently there are only two functions call json::parse_string, when the string is invalid:
json::parse_value->Value::string->json::parse_string, it will also terminate inValue::stringjson::parse_object->json::parse_kv_pair->json::parse_string, it will not terminate, but continue to create a invalid kv_pair injson::parse_kv_pair, and insert it injson::parse_object
I don't think elevating an error into termination is appropriate
I agree with this, but seems json::parse_value and json::parse_object don't behave the same
| val.underlying_ = std::make_unique<ValueImpl>(ValueKindConstant<VK::String>(), std::move(s)); | ||
| return val; | ||
| } | ||
| Value Value::string_valid(std::string&& s) noexcept |
There was a problem hiding this comment.
Unless there is a fairly extreme perf cost from the check on 330 I'm not really seeing a good reason to add this API. If there is an extreme perf reason, perhaps we should consider altering how that assertion works rather than adding a new API footgun like this.
There was a problem hiding this comment.
Yeah, it's not much necessary. I am investigating why sometimes vcpkg search is slow and then find this json::parse_value function
When I pref vcpkg search a, the json::parse_value function only costs about 5%-10%
There was a problem hiding this comment.
I'm guessing you're using manifest mode? In that case the reason search is awfully slow is that we end up launching a git process per port to extract the tree sha. I had some work in progress to use git mktree to replace N git invocations with 2 git invocations but I haven't had time to get back to that.
Also unfortunately child process launch time etc. won't be detected by a sampling based profiler like this since it only cares about CPU time in the process in question.
There was a problem hiding this comment.
I'm using the classic mode, and most of the time vcpkg search takes only < 500ms, but occasionally it's slow, up to 5 seconds I guess
There was a problem hiding this comment.
Did you take your profile during one of the times where it was slow?
There was a problem hiding this comment.
No, currently I can't reproduce when build vcpkg from source
"slow" only occured when using offical vcpkg in classic mode, and not occured on all of my PCs
There was a problem hiding this comment.
I can reproduce when the first time I compile after I reboot, the search method costs about 5s
|
Closed and reopened to force re-merge to pick up the macOS build fix |
2 participants
There are two points to call
json::parse_stringvcpkg-tool/src/vcpkg/base/json.cpp
Lines 910 to 928 in 46b8cce
vcpkg-tool/src/vcpkg/base/json.cpp
Lines 1025 to 1039 in 46b8cce
The current implementation of
json::parse_stringdoesn't exit on invalid strings. So when the string is invalid:json::parse_valueexits but performs redundant string validationjson::parse_kv_paircontinues processing even with an invalid string, which is unnecessary.